-
Notifications
You must be signed in to change notification settings - Fork 740
Release 10.6 final integration #6331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
lehins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't guarantee that my suggestions will compile, but at least they should give you enough info about those new predicate failures.
bc2e58f to
493a96d
Compare
847992c to
c6f48e8
Compare
bench/tx-generator/src/Cardano/Benchmarking/GeneratorTx/SizedMetadata.hs
Outdated
Show resolved
Hide resolved
| Api.AlonzoEraOnwardsBabbage -> renderAlonzoPlutusPurpose | ||
| Api.AlonzoEraOnwardsConway -> renderConwayPlutusPurpose | ||
| -- TODO: fix | ||
| Api.AlonzoEraOnwardsDijkstra -> undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still an undefined in the code
| ] | ||
| forMachine _dtal (TraceLocalRootWaiting d dt) = | ||
| mconcat [ "kind" .= String "LocalRootWaiting" | ||
| -- TODO: `domainAddress` -> `accessPoint` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be addressed in this PR? Should we open a separate issue?
| (M.notMember stakeKeyHash . L.scDeposits) | ||
| (\accountsStates -> isJust $ do | ||
| _state <- M.lookup stakeKeyHash accountsStates | ||
| pure () -- TODO: should we check for balance? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be addressed after fixing testnet.
| _state <- M.lookup stakeKeyHash accountsStates | ||
| pure () -- TODO: should we check for balance? | ||
| ) | ||
| -- (M.notMember stakeKeyHash . L.scDeposits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out instead of removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the previous implementation, to be removed once we fix the testnet tests.
| import qualified Hedgehog as H | ||
| import qualified Hedgehog.Extras as H | ||
|
|
||
| -- TODO we're not supporting non-p2p topology, does this test make any sense now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like this needs to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look. I'll remove this test.
| let disabledTests = pure $ T.testGroup "tests disabled" [] | ||
|
|
||
| E.withArgs args $ tests >>= T.defaultMainWithIngredients T.defaultIngredients | ||
| -- TODO: fix testnet tests and reenable here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just marking this here as the testnet tests TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's out of scope for the node 10.6 release.
| ({ lib, pkgs, ... }: lib.mkIf (pkgs.stdenv.hostPlatform != pkgs.stdenv.buildPlatform) { | ||
| # Remove hsc2hs build-tool dependencies (suitable version will be available as part of the ghc derivation) | ||
| packages.Win32.components.library.build-tools = lib.mkForce [ ]; | ||
| # TODO: error: The option `packages.Win32' does not exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be addressed?
3736db5 to
3aae876
Compare
955a9c8 to
ce90fec
Compare
0ecd14d to
706faab
Compare
Jimbo4350
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with respect to changes that concern my team.
| -- Enable peer to peer discovery | ||
| , ("EnableP2P", Aeson.Bool False) | ||
| -- Enable P2P, non-P2P is gone | ||
| , ("EnableP2P", Aeson.Bool True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If non-P2P is gone why bother keeping the "EnableP2P" configuration key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| (pledgerSAddr, _rewards, _poolId) <- H.headM $ mergeDelegsAndRewards delegsAndRewards | ||
|
|
||
| -- Pledger and owner are and can be the same | ||
| Text.unpack (serialiseAddress pledgerSAddr) === poolownerstakeaddr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why has this check been removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it does not make sense. You have multiple stake addresses in the list in the arbitrary order and it fails randomly. The check that poolownerstakeaddr is in the list is already done in checkStakeKeyRegistered.
| H.renameFile (tempAbsPath' </> "shelley/genesis.json") (tempAbsPath' </> defaultGenesisFilepath ShelleyEra) | ||
| H.renameFile (tempAbsPath' </> "shelley/genesis.alonzo.json") (tempAbsPath' </> defaultGenesisFilepath AlonzoEra) | ||
| H.renameFile (tempAbsPath' </> "shelley/genesis.conway.json") (tempAbsPath' </> defaultGenesisFilepath ConwayEra) | ||
| -- TODO: once 'cardano-cli latest genesis create' supports dijkstra, make this a copy instead of writing a default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carbolymer can you turn this into an issue so we can track it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jutaro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution. I’ve reviewed the changes related to the tracing system and have some important observations:
Incomplete removal of tracing constructors
According to the network changelog, the following were removed:
TraceLedgerPeersResult
TraceLedgerPeersFailure
However, this removal appears to be incomplete. Some references are still present in the codebase, leading to inconsistency and potential confusion during tracing. I will prepare a follow-up commit tomorrow to fully remove the remaining instances and ensure consistency.
TraceChurnMode still present
The changelog also states that:
TraceChurnMode was removed from TracePeerSelection
However, TraceChurnMode still appears in the node codebase—possibly used in a different context now. Before proceeding, I believe the network team should verify that is still required and if the codebase is fully adopted to this change. Otherwise, it may result in dead code or misleading trace output.
Additional tracing inconsistencies
While reviewing this PR, I found several smaller issues and inconsistencies in the trace definitions that should be addressed for correctness and maintainability. I will include these fixes in a follow-up commit tomorrow.
| Api.AlonzoEraOnwardsBabbage -> renderAlonzoPlutusPurpose | ||
| Api.AlonzoEraOnwardsConway -> renderConwayPlutusPurpose | ||
| -- TODO: fix | ||
| Api.AlonzoEraOnwardsDijkstra -> undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still an undefined in the code
ab91132 to
0eaa1e7
Compare
jutaro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All issues I found have been addressed now
4d56bc9 to
0960b09
Compare
Needs #6327
Description
Add your description here, if it fixes a particular issue please provide a
link
to the issue.
Checklist
See Runnings tests for more details
CHANGELOG.mdfor affected package.cabalfiles are updatedhlint. See.github/workflows/check-hlint.ymlto get thehlintversionstylish-haskell. See.github/workflows/stylish-haskell.ymlto get thestylish-haskellversionghc-9.6andghc-9.12Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.